Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Montgomery u32 backend for BabyBear #948

Merged
merged 21 commits into from
Jan 6, 2025
Merged

Conversation

jotabulacios
Copy link
Contributor

@jotabulacios jotabulacios commented Dec 5, 2024

Add Montgomery u32 backend for BabyBear

Description

This PR implements a 32-bit Montgomery arithmetic backend for BabyBear finite field operations.

These operations show significant performance improvements versus the actual u64 backend.
Performance parity with Plonky3 is achieved

Results for 1.000.000 operations

Screenshot 2024-12-05 at 12 54 19 PM

Type of change

  • [ x] Optimization

Checklist

  • [ x] Benchmarks added/run

@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 85.78431% with 58 lines in your changes missing coverage. Please review.

Project coverage is 71.32%. Comparing base (2ce8b2e) to head (06fe23d).

Files with missing lines Patch % Lines
...field/fields/u32_montgomery_backend_prime_field.rs 71.42% 54 Missing ⚠️
math/src/field/fields/fft_friendly/babybear_u32.rs 98.17% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #948      +/-   ##
==========================================
+ Coverage   71.14%   71.32%   +0.17%     
==========================================
  Files         153      155       +2     
  Lines       33129    33537     +408     
==========================================
+ Hits        23570    23920     +350     
- Misses       9559     9617      +58     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jotabulacios jotabulacios marked this pull request as ready for review December 5, 2024 18:01
@jotabulacios jotabulacios requested a review from a team as a code owner December 5, 2024 18:01
///
/// Converts a value from Montgomery domain using reductions mod p
#[inline(always)]
const fn monty_reduce(x: u64, mu: &u32, q: &u32) -> u32 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think better name is montgomery_reduction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Renamed in 32ed5e3


#[inline(always)]
pub const fn mul(a: &u32, b: &u32, q: &u32, mu: &u32) -> u32 {
let x = *a as u64 * *b as u64;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let x = *a as u64 * *b as u64;
let x = u64::from(*a) * u64::from(*b);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We tried to replace as with from but we can't use from inside a constant function.

@ColoCarletti ColoCarletti added this pull request to the merge queue Jan 6, 2025
Merged via the queue into main with commit 4ee0600 Jan 6, 2025
8 checks passed
@ColoCarletti ColoCarletti deleted the add_mont_u32_backend branch January 6, 2025 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants